audio: kpb: Fixing issues reported by CodeQL during PTL code scanning#10858
audio: kpb: Fixing issues reported by CodeQL during PTL code scanning#10858tmleman wants to merge 3 commits into
Conversation
tmleman
requested review from
abonislawski,
kv2019i,
lgirdwood,
serhiy-katsyuba-intel,
softwarecki and
wjablon1
There was a problem hiding this comment.
Pull request overview
This PR addresses CodeQL-reported integer width/overflow concerns in the KPB (Key Phrase Buffer) audio component by widening intermediate types and forcing safer integer promotion during size calculations.
Changes:
- Widened
chloop variables fromuint16_ttouint32_tin mic selection copy helpers. - Added
(size_t)casts in several size/multiplication expressions to avoid unintended 32-bit overflow/truncation in arithmetic.
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
LGTM, but can you resolve the copilot comments. I suspect this pattern/fix will apply in other places too.
tmleman
force-pushed
the
topic/upstream/pr/audio/kpb/fix/CodeQL_issues
branch
from
884f731 to
32254a8
Compare
Six multiplications in the key-phrase buffer compute their product at 32-bit width and only then assign it to a wider size_t result. If the operands are large the overflow has already occurred before widening. The KPB sizing math is partly driven by externally-influenced values (cli->drain_req, the configured channel count, sampling frequency and container width), so this is a real overflow surface rather than a purely theoretical one. Cast the leading operand to size_t in each expression so the whole product is evaluated at the destination width: - kpb_micselect_copy16/32(): loop bound samples_per_chan * in_channels - kpb_init_draining(): drain_req and bytes_per_ms - adjust_drain_interval(): pipeline_period - validate_host_params(): bytes_per_ms No functional change on in-range inputs; only the intermediate arithmetic width changes. Found-by: CodeQL 2.24.2 (codeql/cpp-queries cpp-security-extended), rule cpp/integer-multiplication-cast-to-long. Run with database build-mode=none over sof/src (host clang cannot target the Xtensa production build), 867 files / 98 queries. Findings at kpb.c:1117,1148,1610,1619,1791,2397. AI-triaged: findings manually cross-referenced against clang-tidy bugprone-implicit-widening-of-multiplication-result and semgrep raptor-integer-truncation on the same surface, and confirmed the operand types (uint32_t / macro constants) against struct sof_kpb_config and struct kpb_client before fixing. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
The four kpb_micselect_copy16/32() variants declare the channel loop counter "ch" as uint16_t while iterating "for (ch = 0; ch < ... micsel_channels; ch++)", where micsel_channels is uint32_t. The loop condition promotes ch to a wider type for the comparison, so if micsel_channels ever exceeds UINT16_MAX the counter wraps at 65536 and the loop fails to terminate. The same narrow counter would also truncate channel indexing. Declare ch as uint32_t so its width matches the bound it is compared against and the channel count it indexes. KPB_MAX_MICSEL_CHANNELS keeps the real value small, so this is hardening rather than an observed runaway, but the type mismatch is removed at the source. CodeQL flagged the two non-HiFi variants (kpb.c:1112,1143); the two KPB_HIFI3 variants (kpb.c:1050,1084) carry the identical pattern and are fixed in the same change for consistency. Found-by: CodeQL 2.24.2 (codeql/cpp-queries cpp-security-extended), rule cpp/comparison-with-wider-type. Run with database build-mode=none over sof/src, 867 files / 98 queries. AI-triaged: traced ch and micsel_channels declarations across all four micselect copy functions and confirmed the bound is uint32_t before widening the counter; extended the fix to the HiFi3 variants the tool did not reach in this build configuration. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
In all four kpb_micselect_copy16/32 variants (HiFi3 and generic), the call to buffer_stream_invalidate(source, size) used the output byte count (produced_bytes) as the invalidation size. When in_channels > micsel_channels, the actual input span read by the copy loop is: samples_per_chan * in_channels * sample_size which is larger than the output span (size). Under-invalidating the source buffer on CONFIG_INCOHERENT platforms can cause stale cache reads. Fix: compute samples_per_chan before the invalidate call and pass the correct input span to buffer_stream_invalidate() in all four helpers. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
|
AFAIK |
I'm only fixing/addressing issues reported by one tool. In this case, CodeQL is not aware of the target architecture the code will be built for. It's purely about code quality. KPB is just a mock for us, but that doesn't mean the warnings should be ignored. |
These two commits are addressing issues reported by CodeQL and confirmed by other tools with use of AI for verification.